Walkthrough

This PR consolidates network testing helpers into shared libraries, enhances localnet VM provisioning with runtime command support, introduces new test fixtures for baseline connectivity validation, and refactors existing l2_bridge tests to use the consolidated helpers instead of local implementations.

Changes

Network Test Helper Refactoring and Localnet Test Suite

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• RedHatQE/openshift-virtualization-tests#4962: Refactors tests/network/l2_bridge/nad_ref_change/lib_helpers.py by removing update_nad_references and connectivity helpers that were originally added there, extracting them to shared libraries.
• RedHatQE/openshift-virtualization-tests#4976: Introduces shared tests/network/libs/connectivity.py and tests/network/libs/nad_ref.py modules that consolidate the TCP connectivity and NAD update helpers moved out of the l2_bridge test helpers.

Suggested labels

new-tests

Suggested reviewers

• dshchedr
• orelmisan
• vsibirsk
• RoniKishner
• nirdothan
• EdDev
• frenzyfriday

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

• X
• Mastodon
• Reddit
• LinkedIn

Approvers:

• EdDev
• dshchedr
• myakove
• rnetser
• vsibirsk

Reviewers:

• Anatw
• EdDev
• RoniKishner
• azhivovk
• dshchedr
• frenzyfriday
• nirdothan
• orelmisan
• rnetser
• servolkov
• vsibirsk
• yossisegev

• hold
• verified
• wip
• lgtm
• approve

• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])

• Refactor shared NAD and connectivity helpers for reuse across test packages
• Implement NAD live-update test for localnet with VLAN switching validation
• Add cloud-init runcmd support to localnet_vm for ARP isolation configuration
• Add localnet constants for interface and network naming standardization

Create shared NAD reference update helper

• New file containing update_nad_references() function moved from l2_bridge tests
• Handles atomic VM spec patching for NAD reference updates
• Waits for MigrationRequired condition lifecycle to ensure change completion

libs/net/nad_ref.py

Extract shared helpers to centralized modules

• Remove update_nad_references() function (moved to libs/net/nad_ref.py)
• Remove assert_connectivity() and assert_no_connectivity() functions (moved to
 tests/network/libs/connectivity.py)
• Keep GUEST_IFACE_1 and GUEST_IFACE_2 constants and two_secondary_bridge_vm() function
• Update imports to remove unused dependencies

tests/network/l2_bridge/nad_ref_change/lib_helpers.py

Update imports for refactored helpers

• Update imports to use update_nad_references from libs/net/nad_ref.py
• Update imports to use assert_connectivity and assert_no_connectivity from
 tests/network/libs/connectivity.py
• Remove local helper imports from lib_helpers.py

tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py

Description

The new conftest.py introduces a bare import pytest, which violates the preferred import style
requiring specific imports via from ... import .... This reduces call-site explicitness and makes
it harder to track what is being used from a module.

Code

tests/network/localnet/nad_ref_change/conftest.py[4]

Evidence

PR Compliance ID 7 forbids introducing bare import module statements. The new file
tests/network/localnet/nad_ref_change/conftest.py includes import pytest on line 4.

AGENTS.md: Prefer Specific Imports and Avoid Bare import module
tests/network/localnet/nad_ref_change/conftest.py[4-4]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tests/network/localnet/nad_ref_change/conftest.py` adds a bare `import pytest`, but the compliance rules require avoiding `import module` in favor of specific imports.

## Issue Context
This file only uses `pytest` for the `@pytest.fixture` decorator, so it can be replaced with a direct import (`from pytest import fixture`) and decorator updates.

## Fix Focus Areas
- tests/network/localnet/nad_ref_change/conftest.py[1-50]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

update_nad_references() can block until timeout because wait_for_no_vmi_condition() only returns
when VMI status.conditions is truthy; if conditions becomes None/[] once MigrationRequired clears,
the wait never succeeds. This turns a successful NAD update into a 300s hang/failure.

Code

libs/net/nad_ref.py[R17-24]

Evidence

The new helper always waits for MigrationRequired to disappear, but the underlying wait helper will
not return if status.conditions is empty/None, even though that represents an absent condition per
its own docstring.

libs/net/nad_ref.py[17-24]
libs/net/vmspec.py[251-289]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`wait_for_no_vmi_condition()` incorrectly requires `status.conditions` to be truthy before checking that a condition is absent/False. When the API returns `conditions: []` or `conditions: null` after the condition clears, the function keeps watching until timeout, causing callers like `update_nad_references()` to hang.

### Issue Context
This PR adds `libs/net/nad_ref.update_nad_references()` which always calls `wait_for_no_vmi_condition(vm, "MigrationRequired")` after patching networks.

### Fix Focus Areas
- libs/net/vmspec.py[251-289]
- libs/net/nad_ref.py[17-24]

### Expected fix
- In `wait_for_no_vmi_condition()`, treat `existing_conditions` being `None` or an empty list as success (condition is absent).
- Apply the same logic inside the watcher loop (if `event["object"].status.conditions` is falsy, return).
- (Optional) Add a small unit-ish test or a comment explaining why falsy `conditions` means success.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

update_nad_references() waits for MigrationRequired=True even when the patch makes no effective
change (e.g., network name not found, network has no multus, or NAD name already the same), which
guarantees a timeout. This makes the helper non-idempotent and fragile to minor caller mistakes.

Code

libs/net/nad_ref.py[R18-24]

Evidence

The function unconditionally waits for MigrationRequired after iterating networks, with no
validation that any networkName was modified; therefore, a no-op call will still wait for a
condition that may never occur.

libs/net/nad_ref.py[18-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`update_nad_references()` always calls `wait_for_vmi_condition_status(..., "MigrationRequired")` after patching networks, but it does not verify that any `network.multus.networkName` actually changed. If the update is a no-op, the MigrationRequired condition may never appear and the function will block until timeout.

### Issue Context
This function is now a shared library helper (`libs/net/nad_ref.py`) and will be reused across multiple test suites; it should be robust and idempotent.

### Fix Focus Areas
- libs/net/nad_ref.py[7-24]

### Expected fix
- Track whether at least one network entry is updated (e.g., `changed = True` when setting a different `networkName`).
- If `changed` is False, fail fast with a clear exception (e.g., `ValueError` listing the requested network names) or return early without waiting.
- Optionally, also guard against `vm.template_spec.networks` being None/empty and raise a descriptive error before waiting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

localnet_vm() adds a runcmd parameter but doesn’t document it in the docstring and only applies it
when network_data is provided, so passing runcmd without network_data is a silent no-op. This makes
the helper easy to misuse and harder to maintain.

Code

tests/network/localnet/liblocalnet.py[R69-75]

Evidence

The signature adds runcmd, but the docstring’s Args list does not mention it, and the only usage
is inside the network_data is not None block when constructing cloud-init UserData.

tests/network/localnet/liblocalnet.py[65-139]
tests/network/libs/cloudinit.py[59-73]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`localnet_vm()` gained a `runcmd` parameter, but the function docstring `Args:` section does not describe it, and the implementation only uses it within the `if network_data is not None:` branch. This creates a confusing API contract (callers can pass runcmd and get no effect).

### Issue Context
The new localnet NAD ref change fixtures use `runcmd=ARP_ISOLATION_SYSCTL_CMD` with `network_data`, so current usage works; the issue is about correctness of the helper API and future callers.

### Fix Focus Areas
- tests/network/localnet/liblocalnet.py[65-144]

### Expected fix
- Update the docstring to include `runcmd` and explicitly state it is applied via cloud-init only when `network_data` is provided.
- Optionally, add an explicit guard: if `runcmd` is not None and `network_data` is None, raise `ValueError` to avoid silent misconfiguration.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

CRITICAL: You MUST post an inline review comment on the first changed line of the first file.
The inline comment should contain the full Test Execution Plan (smoke decision, gating decision, and specific affected tests).
Do NOT submit a formal review - just post the inline comment directly.

As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan.
You will post an inline review comment with the test execution plan on the first changed file.
If you fail to run or post a comment, retry.

Analysis Requirements:

1. Examine code changes in each modified file

2. Identify affected code paths, functions, and classes

3. Analyze pytest-specific elements: fixtures (scope, dependencies), parametrization, markers, conftest changes

4. Trace test dependencies through imports, shared utilities, fixture inheritance, fixture teardown, and yield from cleanup in conftest

5. Detect new tests introduced in the PR

6. Utilities and libs impact (when utilities/ or libs/ changes):
   You MUST use shell scripts (rg, git diff) to trace the full impact.
   Follow these sub-steps in order:

   6a. Identify modified symbols: For each changed file under utilities/ or libs/,
   list every modified function or method.
   Example: git diff HEAD~1 --unified=0 -- utilities/hco.py | grep '^[+-]def '

   6b. Find direct callers: Search tests and conftest for each symbol from 6a.
   Example: rg -l 'get_hco_version' tests/

   6c. Trace fixture teardown and cleanup: Find fixtures that reach
   the modified symbol through yield from or context-manager wrappers.
   Example: rg -l 'yield from.*enable_common_boot|def.*enable_common_boot' tests/

   6d. Trace same-file callers: In each changed file, find other functions
   whose body calls a modified symbol (including code after yield
   in @contextmanager helpers).
   Example: rg 'get_hco_version|enable_common_boot' utilities/hco.py

   6e. Expand transitively: If function A calls modified B, then
   tests/fixtures that call A are affected — even when the test body
   never imports B directly.

   Do NOT limit impact to tests that import the modified symbol only.

7. Smoke test impact: Intersect the affected set from step 6 with smoke-marked tests.
   Run: rg -l '@pytest.mark.smoke' tests/
   VERIFY the above command returned actual file paths before concluding False.
   Set True if either condition is met:

   • a smoke-marked file appears in the affected set from 6b-6e, OR
   • any conftest.py in the smoke test's parent-directory hierarchy (up to repo root)
     imports or calls a modified utilities/libs symbol — including autouse fixtures
     that depend on modified functions. ALL tests in that directory and below are affected.
     Example check: for each smoke_file, scan dirname(smoke_file)/conftest.py,
     dirname(dirname(smoke_file))/conftest.py, etc. for modified symbol imports
     and autouse fixtures that depend on modified symbols.

8. Gating test impact: Intersect the affected set from step 6 with gating-marked tests.
   Run: rg -l '@pytest.mark.gating' tests/
   Set True if a gating-marked file also appears in the affected set from 6b-6e.
   Utilities/libs changes often affect gating tests without affecting smoke tests.
   Do NOT stop analysis after concluding Run smoke tests: False.

Output rules:
Do NOT include analysis step numbers (1-8) in your visible output.

Your deliverable:
Your change request comment will be based on the following requirements:

Test Execution Plan

• Run smoke tests: True / False — If True, state the dependency path (test → fixture → changed symbol). True ONLY with a verified path.
• Run gating tests: True / False — If True, state the dependency path. True if any gating-marked test is in the affected set.
• Affected tests to run (required when utilities/, libs/, or shared conftest changes — list concrete paths even when smoke is False)
• path/to/test_file.py - When the entire test file needs verification
• path/to/test_file.py::TestClass::test_method - When specific test(s) needed
• path/to/test_file.py::test_function - When specific test(s) needed
• -m marker - When a marker covers multiple affected tests (e.g. -m gating only if ALL gating tests in scope need run)
• Tag each listed test or group with its marker when not obvious, e.g. (gating) or (smoke)

Guidelines:

• Include tests affected directly OR via fixture setup/teardown, yield from cleanup, or transitive utility call chains (caller calls modified helper)
• Use a full file path only if ALL tests in that file require verification
• Use file path + test name when only specific tests use an affected fixture or utility wrapper (preferred for partial file impact)
• If a test marker can cover multiple files/tests, provide the marker
• Balance coverage vs over-testing - Keep descriptions minimal
• Example: if leaf helper foo() changes, include tests whose fixture teardown calls wrapper bar() where bar() calls foo(), even when the test body only imports an unrelated symbol from the same utilities module

Hardware-Related Checks (SR-IOV, GPU, DPDK):

When PR modifies fixtures for hardware-specific resources:

• Collection Safety: Fixtures MUST have existence checks (return None when hardware unavailable)
• Test Plan: MUST verify both WITH and WITHOUT hardware:
  • Run affected tests on cluster WITH hardware
  • Verify collection succeeds on cluster WITHOUT hardware

CRITICAL WORKFLOW COMPLETION RULES:

When responding to this test execution plan request, you MUST follow these rules EXACTLY:

1. YOUR ONLY DELIVERABLE: Post an inline review comment containing the test execution plan on the first changed line
2. THEN STOP IMMEDIATELY - Do NOT generate any additional response
3. FALLBACK ONLY: If submitting the review fails after retrying, post as a regular PR comment
4. SILENCE = SUCCESS: After successfully submitting the review, your task is complete. No confirmation needed.

ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):

• ❌ Do NOT post acknowledgment messages like "Test execution plan posted", "Review posted successfully", "I've successfully posted"
• ❌ Do NOT mention review IDs, URLs, or confirmation of posting in the PR thread
• ❌ Do NOT add any follow-up comments after submitting the review
• ❌ Do NOT reply to confirm task completion
• ❌ Do NOT explain what you did - just do it and stop

Remember: The pull request review is visible to users. Additional comments are redundant noise.

CRITICAL — Verification rules for smoke and gating decisions:
- For each True decision, you must have traced a concrete path: test file → fixture → utility function → modified symbol.
- Do NOT assume impact based on scope, file proximity, or infrastructure-sounding names.
Only set True when you trace a concrete path through steps 6a-6e, then 7 or 8.
WRONG: "utilities/hco.py changed → run smoke" (no traced dependency path)
WRONG: "Run smoke tests: False" with no affected-test list when gating tests use the changed utility
RIGHT: "Run smoke tests: False, Run gating tests: True" plus tests/.../test_foo.py::TestBar::test_baz (gating)
RIGHT: "Smoke test X uses fixture Y → wrapper Z → changed function W in utilities/hco.py"

🏁 Script executed:

#!/bin/bash
set -e

echo "=== PR files changed ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5039/files \
  --jq '[.[] | .filename]' 2>/dev/null

echo ""
echo "=== Modified symbols: libs/net/nad_ref.py ==="
grep -n "^def " libs/net/nad_ref.py 2>/dev/null || echo "file not found"

echo ""
echo "=== Modified symbols: tests/network/libs/ ==="
ls tests/network/libs/ 2>/dev/null || echo "dir not found"
grep -rn "^def " tests/network/libs/ 2>/dev/null || echo "no defs"

echo ""
echo "=== Modified symbols: tests/network/localnet/liblocalnet.py ==="
grep -n "^def \|^IFACE_A_NAME\|^IFACE_B_NAME\|^CUDN_B_NAME\|^runcmd" tests/network/localnet/liblocalnet.py 2>/dev/null || echo "not found"

echo ""
echo "=== Modified symbols: tests/network/l2_bridge/nad_ref_change/lib_helpers.py ==="
grep -n "^def \|^NET_SEED\|^GUEST_IFACE" tests/network/l2_bridge/nad_ref_change/lib_helpers.py 2>/dev/null || echo "not found"

echo ""
echo "=== 6b: Callers of update_nad_references ==="
rg -rn "update_nad_references" tests/ libs/ 2>/dev/null || echo "none"

echo ""
echo "=== 6b: Callers of localnet_vm ==="
rg -rn "localnet_vm" tests/ 2>/dev/null || echo "none"

echo ""
echo "=== 6b: Callers of assert_connectivity / assert_no_connectivity (current state) ==="
rg -rn "assert_connectivity\|assert_no_connectivity" tests/ 2>/dev/null || echo "none"

echo ""
echo "=== 7: Smoke-marked test files ==="
rg -rl "`@pytest.mark.smoke`" tests/ 2>/dev/null || echo "none"

echo ""
echo "=== 8: Gating-marked test files ==="
rg -rl "`@pytest.mark.gating`" tests/ 2>/dev/null || echo "none"

echo ""
echo "=== Smoke/gating in localnet or l2_bridge paths ==="
rg -rn "`@pytest.mark.smoke`\|`@pytest.mark.gating`" tests/network/localnet/ tests/network/l2_bridge/ 2>/dev/null || echo "none"

echo ""
echo "=== First changed file in PR (for inline comment target) ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5039/files \
  --jq '.[0] | {filename: .filename, first_hunk_pos: (.patch | split("\n") | to_entries | map(select(.value | startswith("+"))) | .[0].key + 1)}' 2>/dev/null || echo "failed"

echo ""
echo "=== Get commit SHA ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5039 --jq '.head.sha' 2>/dev/null

Length of output: 14374

🏁 Script executed:

#!/bin/bash
COMMIT_SHA="7b6aebe8256a87f8551bb44bc1deb413540ae64e"

gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5039/comments \
  --method POST \
  --field body='**Test Execution Plan**

- **Run smoke tests: True** — `tests/network/localnet/test_default_bridge.py` (smoke) → fixtures in `tests/network/localnet/conftest.py` → calls `n()` = `localnet_vm()` in `tests/network/localnet/liblocalnet.py` (modified: new `runcmd` parameter)

- **Run gating tests: False** — No gating-marked test file is in the affected set. All gating-marked tests are in `tests/virt/` and `tests/storage/` paths with no traced dependency on `localnet_vm()` or `update_nad_references`.

**Affected tests to run:**

| Test path | Reason |
|-----------|--------|
| `tests/network/localnet/nad_ref_change/test_nad_ref_change.py` | New test — primary validation target |
| `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py` | Imports `n` (previously `update_nad_references`) from moved `tests/network/libs/nad_ref.py` |
| `tests/network/localnet/test_default_bridge.py` **(smoke)** | Transitive: fixtures in `tests/network/localnet/conftest.py` → `n()` = `localnet_vm()` (modified) |
| `tests/network/localnet/test_ovs_bridge.py` | Transitive: same conftest → `localnet_vm()` (modified) |
| `tests/network/localnet/ipam/` | Transitive: `ipam/conftest.py` → `n()` = `localnet_vm()` (modified) |
| `tests/network/localnet/migration_stuntime/` | Transitive: `migration_stuntime/conftest.py` → `n()` = `localnet_vm()` (modified) |' \
  --field commit_id="$COMMIT_SHA" \
  --field path='tests/network/l2_bridge/nad_ref_change/lib_helpers.py' \
  --field position=1 \
  2>&1

Length of output: 5635